[FIX]: Guard against empty output file path in ServerComparer#14
[FIX]: Guard against empty output file path in ServerComparer#14x15sr71 wants to merge 1 commit into
Conversation
…nt silent comparison failure
| // caller's catch block. | ||
| if (string.IsNullOrEmpty(data.ProducedFile) || !File.Exists(data.ProducedFile)) | ||
| { | ||
| return; |
There was a problem hiding this comment.
If you return without posting a status, how is the platform supposed to know something went wrong?
There was a problem hiding this comment.
The finish POST (exit code + runtime) goes through the same as before this PR, SendExitCodeAndRuntime runs after the comparison loop, outside the try/catch, so neither the old swallowed exception nor the new early return affects it. The TestResult row is created either way; only the equality/upload POST is skipped, so no TestResultFile row is written for that output.
The platform reads that absence as a failure. In get_test_results() (mod_test/controllers.py), when a regression test has no TestResultFile rows it checks RegressionTestOutput directly and sets test_error = True if a non-ignored output was expected (the else branch). That runs on every status surface that calls it, PR status (via comment_pr), PR comment, badge, detail page, except commit-test status, which still uses the old count-based query in progress_type_request where zero rows reads as zero failures → SUCCESS. That's the actual bug; #1119 fixes it by moving that path onto get_test_results().
So this PR is the narrower part: it stops the silent ArgumentException and makes the missing-output path explicit, it doesn't change what's sent to the platform, since finish already fired and the comparison row was already absent before this change. One flag, the guard returns silently, so unlike the old swallowed exception (which hit processor.Logger.Error) there's no tester-side trace now. ServerComparer has no logger field; happy to add a Console.Error.WriteLine in the guard, or wire an ILogger into ServerComparer like Tester and Processor already use - whichever you prefer.
Description
When CCExtractor produces no output file,
ServerComparer.CompareAndAddToResult()callsFile.Open("", FileMode.Open), which throwsArgumentException. The caller inTester.csswallows it and unconditionally callsSendExitCodeAndRuntime()— so the test is marked finished with noTestResultFilerow written and no comparison data sent to the platform.Execution chain
Findings
Command run on the sample-platform server:
Logs —
[ERROR] Path is emptyimmediately followed by[INFO] Finished entry Xin 10 production log files (4778, 4782, 4790, 4792, 4794, 4798, 4804, 4828, 5124, 9298). Example from log 9298 (25 occurrences,
wc -lverified):Log 9299 (same test, Windows): 0 occurrences — Windows missing rows have a separate unrelated cause not addressed here.
DB —
test_resultrow exists (finish received) but notest_result_filerow (comparison never ran),exit_code=0,expected_rc=0, excluding ignored/stdout tests (139, 238, 239):Counts tests where finish was received but no comparison row was written — includes but is not limited to instances caused by this bug.
11 affected regression test IDs (7, 16, 138, 140, 143, 144, 145, 148, 150, 154, 155) all have thousands of historical
TestResultFilerows, confirming this is a runtime failure not a data model gap.Fix
Add a guard inside the
!Hasher.filesAreEqualbranch that checksstring.IsNullOrEmpty(data.ProducedFile) || !File.Exists(data.ProducedFile)and returns early, preventingFile.Open("")from being called.Impact
ArgumentExceptionswallowedTestResultFilerow[ERROR]in logRisks
TestResultFilerow — that requiresserver-side changes and is out of scope.
sample-platformaddresses the counting logic.Deployment
Independent of
sample-platform. Can deploy in parallel with #1118.